Conversation
| ObjectPool<XdsClient> getOrCreate( | ||
| String target, BootstrapInfo bootstrapInfo, MetricRecorder metricRecorder, | ||
| ManagedChannel parentChannel); | ||
|
|
There was a problem hiding this comment.
Should this be a generic object instead of ManagedChannel ?
| return new XdsServerWrapper("0.0.0.0:" + port, delegate, xdsServingStatusListener, | ||
| filterChainSelectorManager, xdsClientPoolFactory, bootstrapOverride, filterRegistry); | ||
| filterChainSelectorManager, xdsClientPoolFactory, bootstrapOverride, filterRegistry, | ||
| this.childChannelConfigurer); |
There was a problem hiding this comment.
Why does this not pass the builder object ?
| @@ -0,0 +1,70 @@ | |||
| /* | |||
| * Copyright 2025 The gRPC Authors | |||
| * universal configuration methods (like {@code intercept()}, {@code userAgent()}) rather | ||
| * than casting to specific implementation types. |
There was a problem hiding this comment.
| * universal configuration methods (like {@code intercept()}, {@code userAgent()}) rather | |
| * than casting to specific implementation types. | |
| * universal configuration methods (like {@code intercept()}, {@code userAgent()}) on the builder rather | |
| * than casting it to specific implementation types. |
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
I know there will be changes based on the gRFC discussion, but here are some things I noticed.
| * | ||
| * @param delegate the configurer to wrap. | ||
| */ | ||
| public static ChildChannelConfigurer safe(ChildChannelConfigurer delegate) { |
There was a problem hiding this comment.
If it is throwing, then something very wrong is happening. How does whoever is using this know it is safe to ignore? Note that the builder can be half-configured, so the built channel may be broken. This seems like a bad idea.
| * @param condition true to apply the delegate, false to do nothing. | ||
| * @param delegate the configurer to apply if condition is true. | ||
| */ | ||
| public static ChildChannelConfigurer conditional(boolean condition, |
There was a problem hiding this comment.
This is an if. Given how simple this implementation is, is it really better than a user doing the ternary operator themselves? It seems like using this would reduce readability.
| * @since 1.79.0 | ||
| */ | ||
| @Internal | ||
| public ChildChannelConfigurer getChildChannelConfigurer() { |
There was a problem hiding this comment.
No, we will want to pass the configurer explicitly down in our APIs. It is weird to add an internal API to access the Channel only so that some other internal getter API can be accessed. That is very indirect. We can just return the final object directly.
| * @since 1.79.0 | ||
| */ | ||
| @Internal | ||
| public ChildChannelConfigurer getChildChannelConfigurer() { |
There was a problem hiding this comment.
This will be unused. Delete it. XdsServerBuilder needs it, but it has access to the value already.
3 participants
No description provided.